Compute quoted args for debuginfo at most once per session #146973
Conversation
|
Some changes occurred in compiler/rustc_codegen_ssa |
rustbot
added
A-LLVM
rustbot
added
the
T-compiler
|
r? @SparrowLii rustbot has assigned @SparrowLii. Use |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
Compute quoted args for debuginfo at most once per session
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
| /// forms of debuginfo (specifically PDB). | ||
| /// | ||
| /// This needs to _not_ be a query, because it would ruin incremental | ||
| /// compilation for CGUs that would otherwise be considered up-to-date. |
There was a problem hiding this comment.
If the commandline changed, then we should recompile all CGUs as otherwise the commandline arguments in the debuginfo would be wrong. If it doesn't change, then no CGUs would be recompiled even if it was a query. If unconditionally recompiling cgus when the cli changes is not acceptable, then maybe this entire feature should be put behind a cli flag and be disabled by default?
There was a problem hiding this comment.
As far as I can tell, bypassing the query system is already the current behaviour. So if that's a problem, maybe the whole “command-line in PDB” thing needs to be ripped out until it can be re-landed in an acceptable way.
(I don't have any attachment to the feature myself; I'm just trying to make the compiler do this quoting step once per process instead of literally 500+ times for no good reason.)
There was a problem hiding this comment.
As far as I can tell, bypassing the query system is already the current behaviour.
Yeah, I don't think anyone considered this when landing the original version.
There was a problem hiding this comment.
There's also the semi-related #128842, where the EXE path being embedded in PDB is reportedly troublesome.
There was a problem hiding this comment.
Yeah, I guess we should either rip out this feature or put it behind a flag.
There was a problem hiding this comment.
I opened https://rust-lang.zulipchat.com/#narrow/channel/131828-t-compiler/topic/Some.20PDB.20info.20bypasses.20the.20query.20system.20and.20path.20remapping/with/541369247 to ask if anyone else has opinions.
My preference is to just rip out the whole thing and wait for someone to complain, since it's having outsized impact relative to its niche use-case, and we have no idea whether anyone is actually benefiting from it.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (ffa991c): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -1.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary -2.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 471.082s -> 469.259s (-0.39%) |
rustbot
removed
the
S-waiting-on-perf
|
Closing this while we see how #147022 shakes out. |
rustbot
removed
the
S-waiting-on-review
EDIT: There is now a proposal to remove the underlying code entirely, which would make this PR obsolete.
Via #146700 and #146845, it was discovered that we were building multiple identical copies of these quoted command-line args for every CGU, instead of just building them 0-1 times for the whole compilation session.
Memoizing them turned out to be harder than expected, for two reasons:
This PR therefore computes the quoted args via a hook, backed by an explicit OnceLock stored in TyCtxt.
One of the side-effects of this PR is that we also no longer supply command-line arguments when creating an informational target machine; we only supply them when creating one that will (presumably) be used for actual codegen.